Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(content, port): Port vitamin-based allergies and cannibalism from DDA #5808

Merged
merged 13 commits into from
Dec 17, 2024

Conversation

RobbieNeko
Copy link
Contributor

@RobbieNeko RobbieNeko commented Dec 13, 2024

Checklist

Required

Optional

  • This PR ports commits from DDA or other cataclysm forks.
    • I have attributed original authors in the commit messages adding Co-Authored-By in the commit message.
    • I have linked the URL of original PR(s) in the description.
  • This is a C++ PR that modifies JSON loading or behavior.
    • If documentation for this feature does not exist, please write it or at least note its lack in PR description.

Purpose of change

Fixes #5613 and many requests for them to be ported by porting the following DDA PRs:
Cannibalism checks as a vitamin
Vitamin-based allergy

This should greatly improve user experience for anyone with traits regarding dietary restrictions, and also in theory allows us to cut back on any bloated food variants that are just about providing options for certain dietary restrictions.

Describe the solution

  • Ports the PRs mentioned above
  • Uses the same script as used for the latter DDA PR in order to apply the vitamins for allergies automatically (a link to the repo for said script can be found in said DDA PR)
  • Enforces the convention of materials always being contained in an array for food-related materials
    • This is necessary for the script to function, and also just a nice change for consistency.

Describe alternatives you've considered

  • Implode
  • Reinvent the wheel
  • Wait for someone else to do it
  • Leave out the formatting change and instead create an altered version of the script
    • While more-atomic commits are nice, I don't see the harm in doing a bit of necessary formatting here when it's as easy as some find-replace work. Besides, standardization is good. Also, this way I don't have to learn typescript or make an entirely new python script for this

Testing

  • It builds on my machine
  • It loads (after making condition_type an enum class so that we don't have namespace collision with vitamins.h VITAMIN), and it indeed behaves appropriately with dietary restrictions!
  • It works without Simplified Nutrition as well, which is good.

Herbivores can't eat meat:
image

Carnivores can't eat veggies:
image

It works without Simplified Nutrition:
image

Additional context

Basic documentation added at request of Scarf
Haven't tested Cannibalism yet, but I assume it works

RobbieNeko and others added 7 commits December 11, 2024 21:14
Oops, didn't realize that the script for the other vitamins didn't include cannibalism

Co-Authored-By: RenechCDDA <[email protected]>
Co-Authored-By: Antti Riikonen <[email protected]>
Enforces the idea that materials should always be in arrays even if there's only one, specifically for the food-related materials. This is necessary for the vitamins script to work correctly.

Also does that 2nd pass of the vitamins script

Co-Authored-By: Antti Riikonen <[email protected]>
@github-actions github-actions bot added src changes related to source code. JSON related to game datas in JSON format. mods PR changes related to mods. labels Dec 13, 2024
@github-actions github-actions bot added the tests changes related to tests label Dec 13, 2024
The DDA script apparently just didn't touch nuts?
We don't even HAVE bread material!
@github-actions github-actions bot added the docs PRs releated to docs page label Dec 14, 2024
Copy link
Contributor

autofix-ci bot commented Dec 14, 2024

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

@RobbieNeko
Copy link
Contributor Author

Thank you Tree of formatting

@RobbieNeko
Copy link
Contributor Author

image
The Windows build is attempting to pocket-veto the PR via refusing to actually end tests lmao

Copy link
Collaborator

@RoyalFox2140 RoyalFox2140 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine, it seems to work. But This needs more reviewers. It's 2000 lines.

@KheirFerrum
Copy link
Collaborator

KheirFerrum commented Dec 16, 2024

I'm looking at the changes to the "material" definitions in the JSONs and wondering if anything done to the cpp necessitates it.

If it does, I'll need to suggest some changes to the cpp to use assigns or something to maintain the old method of defining materials, otherwise this will require a lot of tedious rewriting of JSONs for no reason in every mod available to BN.

If it doesn't, then I'd suggest removing those changes to the JSON as they are not necessary and may end up confusing.

Edit: I've been told the changes are purely JSON side and are necessary for a script to add nutrients to food. As this doesn't change how the game loads cpp older 1-entry material definitions will still function so I'm good with it.

@Zlorthishen
Copy link
Contributor

Zlorthishen commented Dec 16, 2024

I compiled this locally and it works for Windows 11.

I confirmed that:

  • Herbivores refused to eat meat
  • Carnivores refused to eat any plant
  • Hates Fruit refused to eat any fruit, but would eat other plants without the allergen_fruit
  • Cannable works
    image

@RoyalFox2140
Copy link
Collaborator

I think this is safe to merge. Kheir and Scarf have both looked at it somewhat, it's been compile tested a few times. We can fix anything missed later in post.

@RoyalFox2140 RoyalFox2140 merged commit 43d2a72 into cataclysmbnteam:main Dec 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PRs releated to docs page JSON related to game datas in JSON format. mods PR changes related to mods. src changes related to source code. tests changes related to tests
Projects
None yet
5 participants